Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Modularize beacon node backend #4718

Open
wants to merge 111 commits into
base: unstable
Choose a base branch
from
Open

Modularize beacon node backend #4718

wants to merge 111 commits into from
+1,563 −632

Conversation

eserilev
Copy link
Collaborator

@eserilev eserilev commented Sep 9, 2023 •
edited
Loading

Issue Addressed

#4669

Proposed Changes

Modularize the beacon node backend to make it easier to add new database implementations

Additional Info

The existing KeyValueStore trait already does some abstraction, however the codebases assumes the KV store is always LevelDB.

I created a BeaconNodeBackend type that implements the KeyValueStore and ItemStore traits. I then replaced all references of LevelDb to the new BeaconNodeBackend type. Within the BeaconNodeBackend type I used the cfg macro which should allow us to switch between different database implementations via config changes.

@eserilev eserilev changed the title [WIP] Modularize beacon node backend Modularize beacon node backend Sep 9, 2023
@michaelsproul michaelsproul mentioned this pull request Sep 29, 2023
Closed
@jimmygchen jimmygchen added ready-for-review The code is ready for review database labels Oct 18, 2023
@eserilev
Copy link
Collaborator Author

eserilev commented Jan 30, 2024 •
edited
Loading

I'm currently working on adding Redb to the beacon node backend. Here are some notes I've taken so far on my work

Redb

Theres a few difference's between LevelDB and Redb that I'll describe below

fsync

Info about redb transaction durability can be found here
https://docs.rs/redb/latest/redb/enum.Durability.html#

For now I translate WriteOptions.sync = true to be Durability::Immediate and WriteOptions.sync = false to be Durability::Eventual

We could decide to use Paranoid instead of Immediate, but I think the additional guarantees of Paranoid are probably unecessary for our use case.

Compaction

LevelDB allows for compacting over a range of values. Redb only allows for compacting the full db. It seems like our LevelDB implementation compacts across all keys in the DB. If thats the case we should be all good here.

Tables

Redb introduces the concept of tables. We can either use one table for everything, or spin up tables by column name. I'm currently going down the path of spinning up tables by column name. This causes issues with do_atomically. We may need to make changes to this function to also accept a col: &str as an argument.

@eserilev eserilev mentioned this pull request Aug 30, 2024
Open
@eserilev
Copy link
Collaborator Author

I've opened a separate issue around temp state cleanup performance for Redb. I'm going to handle that in a separate PR if thats ok with you guys

#6332

@eserilev
Copy link
Collaborator Author

eserilev commented Sep 1, 2024

Compaction in redb v2.1.2 is craaazy slow: cberner/redb#852

For now lets just make sure we stay on 2.1.1 until the issue is resolved

@eserilev
Copy link
Collaborator Author

the last compaction run took 7 min to complete

image

I think we were running an older version of Redb that had introduced a regression to compaction. I'm really hoping the new version of Redb, which fixed the regression and introduced some optimizations, will improve those times. We should see compaction run on gown on sep 23rd.. will update here once that runs.

If compaction speeds still suck, one option is to have two separate databases for redb. The first contains the tables we dont want to run compaction for, the second contains tables we do want to run compaction for. That way we can run compaction against these three tables:

  DBColumn::BeaconState,
  DBColumn::BeaconStateSummary,
  DBColumn::BeaconBlock,

And not against the full set of tables (mimicking leveldb functionality).

@eserilev eserilev mentioned this pull request Nov 7, 2024
Open
michaelsproul
michaelsproul reviewed Nov 29, 2024
View reviewed changes
beacon_node/store/src/lib.rs
Comment on lines +192 to +195
PutKeyValue(String, Vec<u8>, Vec<u8>),
// Indicate that a DELETE operation should be made
// to the db store for a (Column, Key)
DeleteKey(String, Vec<u8>),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use DBColumn here directly, or failing that &'static str. DBColumn has the advantage of being type safe and saving heap allocations, while &'static str just saves heap allocations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dapplion dapplion dapplion left review comments

@michaelsproul michaelsproul michaelsproul left review comments

Assignees
No one assigned
Labels
database ready-for-review The code is ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@eserilev @jimmygchen @michaelsproul @dapplion